Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added background to CommandKeyChord #15677

Conversation

RickleAndMortimer
Copy link
Contributor

@RickleAndMortimer RickleAndMortimer commented Jul 7, 2023

Summary of the Pull Request

Adds a background to key chord border in the CommandPalette Screen. This prevents certain accent colors from rendering the KeyChords unreadable.

Before (where the text is unreadble);
image

After (from this PR):
image

See #15228 for more details

Closes #15228

@lhecker
Copy link
Member

lhecker commented Jul 10, 2023

While this is an acceptable solution, why can't we make the border and text within black, similar to how "Close pane" turns from white to black when being hovered?

@RickleAndMortimer
Copy link
Contributor Author

RickleAndMortimer commented Jul 11, 2023

This was the approach that was suggested here #15228 (comment)

Note

Walkthrough

Probably the easiest way to do this would be to just add a .Background() to this border:

<Border Grid.Column="2"
Padding="2,0,2,0"
HorizontalAlignment="Right"
VerticalAlignment="Center"
AutomationProperties.AccessibilityView="Raw"
Style="{ThemeResource KeyChordBorderStyle}"
Visibility="{x:Bind Item.KeyChordText, Mode=OneWay, Converter={StaticResource CommandKeyChordVisibilityConverter}}">

and use like, the CommandPalette background there, so that it appears transparent, but actually isn't.
(this was an off the cuff suggestion during triage, and we all thought it was a good idea)

I can definitely take a look at coloring the borders and text black instead, looks like a good idea as well 👍

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly, I'm cool with this as-is. If you can figure out how to get it to match the other text, be my guest, but this alone is still an improvement.

@lhecker
Copy link
Member

lhecker commented Jul 12, 2023

It's a ✅ for me as well, but I'll wait a little bit just in case you happen to have time to check if it can be done with the color change, just because I think it would look better / more consistent. But I'd definitely also approve it without such a change.

@RickleAndMortimer
Copy link
Contributor Author

Not sure if I can find a clean approach in changing colors based on the theme. This is good to merge 👍 as is

@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 17, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot merged commit 91012a4 into microsoft:main Jul 17, 2023
@DHowett
Copy link
Member

DHowett commented Jul 17, 2023

Thanks for the contribution! 😄

DHowett pushed a commit that referenced this pull request Aug 2, 2023
## Summary of the Pull Request

Adds a background to key chord border in the CommandPalette Screen. This
prevents certain accent colors from rendering the KeyChords unreadable.

Before (where the text is unreadble);

![image](https://github.com/microsoft/terminal/assets/33658638/370fa7c7-f42e-48b3-af54-6fe7d5f89c73)

After (from this PR):

![image](https://github.com/microsoft/terminal/assets/33658638/5ce8601a-80f2-4efe-9270-9dd7209cdfff)

See #15228 for more details

(cherry picked from commit 91012a4)
Service-Card-Id: 90068187
Service-Version: 1.17
DHowett pushed a commit that referenced this pull request Aug 11, 2023
## Summary of the Pull Request

Adds a background to key chord border in the CommandPalette Screen. This
prevents certain accent colors from rendering the KeyChords unreadable.

Before (where the text is unreadble);

![image](https://github.com/microsoft/terminal/assets/33658638/370fa7c7-f42e-48b3-af54-6fe7d5f89c73)

After (from this PR):

![image](https://github.com/microsoft/terminal/assets/33658638/5ce8601a-80f2-4efe-9270-9dd7209cdfff)

See #15228 for more details

(cherry picked from commit 91012a4)
Service-Card-Id: 90068188
Service-Version: 1.18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Marked for automatic merge by the bot when requirements are met
Projects
Development

Successfully merging this pull request may close these issues.

Command Palette key bindings are unreadable in Dark theme with light accent color
5 participants